Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[drake_bazel_external] Port to use MODULE.bazel #348

Merged

Conversation

jwnimmer-tri
Copy link
Contributor

@jwnimmer-tri jwnimmer-tri commented Dec 20, 2024

Towards RobotLocomotion/drake#20731.


This change is Reviewable

Copy link
Contributor Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, platform LGTM missing (waiting on @jwnimmer-tri)


drake_bazel_external/MODULE.bazel line 35 at r2 (raw file):

    module_name = "drake",
    urls = [x.format(DRAKE_COMMIT) for x in [
        "https://github.com/jwnimmer-tri/drake/archive/{}.tar.gz",

Working

Revert after the Drake PR lands.

@SeanCurtis-TRI SeanCurtis-TRI self-assigned this Jan 7, 2025
Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+@SeanCurtis-TRI for feature review.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee seancurtis-tri, platform LGTM missing (waiting on @jwnimmer-tri)

@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review January 7, 2025 22:16
Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:LGTM: (pending removal of temporary Drake stub)

Reviewed 7 of 8 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, platform LGTM from [seancurtis-tri] (waiting on @jwnimmer-tri)

Copy link
Contributor Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 unresolved discussions, platform LGTM from [seancurtis-tri] (waiting on @jwnimmer-tri)


drake_bazel_external/apps/BUILD.bazel line 83 at r2 (raw file):

pybind_py_library(
    name = "simple_adder_py",
    cc_binary_rule = cc_binary,

Working

Ah, I'd forgotten about this change. Since I originally wrote it, I've tried to make further edits to Drake that so that the tweaks here are no longer necessary. I'll play around to see if I can undo all changes to this file.

@jwnimmer-tri jwnimmer-tri force-pushed the bazel-external-bzlmod branch from aa2c55c to 751556e Compare January 7, 2025 22:54
Copy link
Contributor Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, platform LGTM from [seancurtis-tri] (waiting on @SeanCurtis-TRI)


drake_bazel_external/apps/BUILD.bazel line 83 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

Ah, I'd forgotten about this change. Since I originally wrote it, I've tried to make further edits to Drake that so that the tweaks here are no longer necessary. I'll play around to see if I can undo all changes to this file.

Yay, no changes actually needed for user BUILD files. Reverted.

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion, platform LGTM from [seancurtis-tri] (waiting on @jwnimmer-tri)

@jwnimmer-tri jwnimmer-tri force-pushed the bazel-external-bzlmod branch from 751556e to d7feb1a Compare January 8, 2025 23:56
Copy link
Contributor Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [seancurtis-tri] (waiting on @SeanCurtis-TRI)


drake_bazel_external/MODULE.bazel line 35 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

Revert after the Drake PR lands.

Done

@jwnimmer-tri
Copy link
Contributor Author

+@ggould-tri for platform review, please.

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm_strong: It's great to see this!

Reviewed 5 of 8 files at r1, 1 of 2 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion, platform LGTM from [seancurtis-tri, ggould-tri] (waiting on @jwnimmer-tri)


drake_bazel_external/MODULE.bazel line 16 at r4 (raw file):

# By default, this example always uses the latest Drake master branch.
DRAKE_COMMIT = "master"
DRAKE_CHECKSUM = ""

BTW: Although I'm confident that this is correct, I can't find it documented anywhere that this is the correct "don't care" default sentinel value.

@jwnimmer-tri jwnimmer-tri force-pushed the bazel-external-bzlmod branch from 76257f4 to 395e482 Compare January 9, 2025 17:57
Copy link
Contributor Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I added one new snippet for macOS support. It's not covered by DEE CI (we don't officially support drake_bazel_external on macOS) but RobotLocomotion/drake#22435 reminded me it would be necessary.

Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [seancurtis-tri, ggould-tri] (waiting on @ggould-tri)


drake_bazel_external/MODULE.bazel line 16 at r4 (raw file):

Previously, ggould-tri wrote…

BTW: Although I'm confident that this is correct, I can't find it documented anywhere that this is the correct "don't care" default sentinel value.

I can't fine any either, but anyway probably using None is epsilon less worrysome.

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [seancurtis-tri, ggould-tri] (waiting on @jwnimmer-tri)

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [seancurtis-tri, ggould-tri] (waiting on @jwnimmer-tri)

@jwnimmer-tri jwnimmer-tri merged commit 431d6d2 into RobotLocomotion:main Jan 10, 2025
7 checks passed
@jwnimmer-tri jwnimmer-tri deleted the bazel-external-bzlmod branch January 10, 2025 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants